Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/gnrc/tcp: fix invalid read #11999

Merged
merged 1 commit into from
Aug 20, 2019
Merged

net/gnrc/tcp: fix invalid read #11999

merged 1 commit into from
Aug 20, 2019

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Aug 12, 2019 •
edited
Loading

Contribution description

From the gnrc_pktbuf_mark documentation:

    It's not guaranteed that `result->data` points to the same address as the original `pkt->data.

Thus it should be necessary to update the hdr pointer.

Testing procedure

  1. Add USEMODULE += gnrc_pktbuf_malloc to tests/gnrc_tcp_server/Makefile
  2. Compile gnrc_tcp_server using make -C tests/gnrc_tcp_server/ all-valgrind
  3. Start gnrc_tcp_server using make -C tests/gnrc_tcp_server/ term-valgrind
  4. Send a short packet e.g. using echo f | nc <ip> <port>

Expect output: Valgrind shouldn't report any out-out-bounds memory access.

Actual output: Valgrind reports an Invalid read of size 2 during checksum comparison.

==4765== Invalid read of size 2
==4765==    at 0x114859: _receive (sys/net/gnrc/transport_layer/tcp/gnrc_tcp_eventloop.c:168)
==4765==    by 0x114B01: _event_loop (sys/net/gnrc/transport_layer/tcp/gnrc_tcp_eventloop.c:255)
==4765==    by 0x499D53A: makecontext (/build/glibc-Stc26X/glibc-2.28/stdlib/../sysdeps/unix/sysv/linux/i386/makecontext.S:91)
==4765==  Address 0x4b41ac0 is 16 bytes inside a block of size 22 free'd
==4765==    at 0x4836C47: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-x86-linux.so)
==4765==    by 0x110853: _mark (sys/net/gnrc/pktbuf_malloc/gnrc_pktbuf_malloc.c:134)
==4765==    by 0x11092C: gnrc_pktbuf_mark (sys/net/gnrc/pktbuf_malloc/gnrc_pktbuf_malloc.c:153)
==4765==    by 0x114824: _receive (sys/net/gnrc/transport_layer/tcp/gnrc_tcp_eventloop.c:158)
==4765==    by 0x114B01: _event_loop (sys/net/gnrc/transport_layer/tcp/gnrc_tcp_eventloop.c:255)
==4765==    by 0x499D53A: makecontext (/build/glibc-Stc26X/glibc-2.28/stdlib/../sysdeps/unix/sysv/linux/i386/makecontext.S:91)
==4765==  Block was alloc'd at
==4765==    at 0x483463B: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-x86-linux.so)
==4765==    by 0x1107F9: _mark (sys/net/gnrc/pktbuf_malloc/gnrc_pktbuf_malloc.c:127)
==4765==    by 0x11092C: gnrc_pktbuf_mark (sys/net/gnrc/pktbuf_malloc/gnrc_pktbuf_malloc.c:153)
==4765==    by 0x11C181: _receive (sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c:706)
==4765==    by 0x11B5F9: _event_loop (sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c:188)
==4765==    by 0x499D53A: makecontext (/build/glibc-Stc26X/glibc-2.28/stdlib/../sysdeps/unix/sysv/linux/i386/makecontext.S:91)

@nmeum
net/gnrc/tcp: fix uninitialized memory access
9e91d21 
From the gnrc_pktbuf_mark documentation:

	    It's not guaranteed that `result->data` points to the
	    same address as the original `pkt->data.

Thus it should be necessary to update the `hdr` pointer.
@nmeum nmeum mentioned this pull request Aug 12, 2019
Merged
@fjmolinas fjmolinas added Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 13, 2019
@fjmolinas fjmolinas requested a review from miri64 August 13, 2019 06:24
brummer-simon
brummer-simon approved these changes Aug 17, 2019
View reviewed changes
Copy link
Member

@brummer-simon brummer-simon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine by me. Thanks for finding it.

@PeterKietzmann PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 19, 2019
@PeterKietzmann
Copy link
Member

PR passed CI. @brummer-simon have you tested this PR? Please set the respective "Reviewed..." labels and approve

@brummer-simon
Copy link
Member

@PeterKietzmann I can't add any labels because I have no maintainer status.

I tried to reproduce the Issue by following the given steps (more details on the tap device setup would be nice) but failed. However I verified the must be updated due to the 'gnrc_pktbuf_mark' documentation.

In case @nmeum gives more information on the bug reproduction, I would like to verify the PR.

@nmeum
Copy link
Member Author

nmeum commented Aug 19, 2019 •
edited
Loading

I tried to reproduce the Issue by following the given steps (more details on the tap device setup would be nice) but failed.

What details do you need regarding the tap setup? This should be reproducible independently of any tap device configuration but since this is might be undefined behavior you never know. In any case, it would be nice to know what exactly failed. See also: #12001 (comment)

@brummer-simon
Copy link
Member

brummer-simon commented Aug 19, 2019 •
edited
Loading

I had to apply this PR for testing #12001. Without application of this PR the valgrind output of the test in #12001 is "Invalid read of size 2". By adding this PR the error is gone.

I would says that this tested the fix although is was not able to reproduce it with the given test: "echo f | nc " could not resolve the given IP Address but it might be my local setup.

All in all I think this is PR fine but I don't have maintainer status so I can't add any labels.

benpicco
benpicco approved these changes Aug 19, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix has been tested & approved by the author of the module.

@benpicco benpicco merged commit 20dec77 into RIOT-OS:master Aug 20, 2019
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
@nmeum nmeum mentioned this pull request Jan 17, 2020
Merged
@brummer-simon brummer-simon mentioned this pull request Oct 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@brummer-simon brummer-simon brummer-simon approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nmeum @PeterKietzmann @brummer-simon @benpicco @kb2ma @fjmolinas